Skip to content

Conversation

@testn
Copy link
Contributor

@testn testn commented Oct 7, 2021

Avoid leaking TextRow/BinaryRow object outside the framework by
returning a new object every time instead of returning this

@sidorares
Copy link
Owner

have you tried benchmarking it (select rows with bunch of fields and also access data in every returned field)
There might be potentially an impact on inline cache for object properties in case of generic object and object where constructor does this.propName = value

@sidorares
Copy link
Owner

Researching if it would be helpful to have per PR automatic benchmark action - do you have an experience with that @testn ?

Maybe https://github.com/benchmark-action/github-action-benchmark

@testn
Copy link
Contributor Author

testn commented Oct 7, 2021

@sidorares

have you tried benchmarking it (select rows with bunch of fields and also access data in every returned field) There might be potentially an impact on inline cache for object properties in case of generic object and object where constructor does this.propName = value

it is quite tough to benchmark this on my machine somehow. The crude benchmark I tried has very high variance (+/-70%). Can you try this and see if you see any diff?

@testn
Copy link
Contributor Author

testn commented Oct 8, 2021

re: accessing every field, sequelize currently copies every field from TextRow object into its own object. Therefore, every field is already accessed.

@sidorares
Copy link
Owner

I'll try to benchmark locally, also wonder how reliable numbers are when run inside GH action

@testn
Copy link
Contributor Author

testn commented Oct 9, 2021

I actually tried to optimize this one a little further by creating it as a class instead where it will cache the typeCast field information for repeated use. The generated code will look something like this

(function anonymous(
) {
return ((function () {
  return class TextRow {
    constructor() {
      const _this = this;
      this.wrap0 = {
        type: "VAR_STRING",
        length: "24",
        db: "",
        table: "",
        name: "foo",
        string: function() {
          return _this.packet.readLengthCodedString("utf8");
        },
        buffer: function() {
          return _this.packet.readLengthCodedBuffer();
        },
        geometry: function() {
          return _this.packet.parseGeometryValue();
        },
        readNext: function() {
          return _this.packet.readLengthCodedString("utf8");
        }
      };
    }
    next(packet, fields, options) {
      this.packet = packet;
      const result = {};
      // "foo": VAR_STRING
      result["foo"] = options.typeCast(this.wrap0, this.wrap0.readNext);
      return result;
    }
  };
})())
})

@testn
Copy link
Contributor Author

testn commented Oct 9, 2021

With this, typeCast query that returns 1,000 rows can yield up to 5-15% gain.

@sidorares
Copy link
Owner

FYI @testn - I added benchmark workflow in #1406 but as far as I understand the way it works it compares benchmarks from current run to the previous one and I see big variations in results even with no changes in the code. I'll see if I can change it ( instead of saving bench results in the cache for later access, always perform 2 benchmarks in the same run, one for master and one for current commit ). If performance alert comments become too noisy I'll disable them

@sidorares
Copy link
Owner

could you rebase your branch so it'll run same checks as in master? I removed travis and appveyor in favour of GH actions

@testn
Copy link
Contributor Author

testn commented Oct 11, 2021

@sidorares Let me do that.

@testn
Copy link
Contributor Author

testn commented Oct 11, 2021

@sidorares done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants